Skip to content

Conversation

@Shehrozkashif
Copy link
Collaborator

@Shehrozkashif Shehrozkashif commented Apr 16, 2025

This pr includes csr vsip,hegeie,hideleg,hie,hip,hvip,vsie,vsscratch,hgeip

@ThinkOpenly ThinkOpenly changed the title Add H csr vsip,hegeie,hideleg,hie,hip,hvip,vsie,vsscratch,hgeip Add H csr vsip,hgeie,hideleg,hie,hip,hvip,vsie,vsscratch,hgeip Apr 16, 2025
Copy link
Collaborator

@ThinkOpenly ThinkOpenly left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did not finish the review, but thought this feedback would be helpful for now.

@Shehrozkashif Shehrozkashif force-pushed the H_CSRs branch 2 times, most recently from ef52648 to 0e1a7fb Compare April 29, 2025 05:08
Copy link
Collaborator

@dhower-qc dhower-qc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a complicated set of CSRs. I've added a bunch of comments, but note that we're likely to need multiple review passes before it's ready.

Comment on lines 32 to 41
VSTIP:
location: 6
type: RO
reset_value: 0
description: |
Pending interrupt bit for VS-level timer interrupts (VSTI).
This bit is the logical OR of `vstip` from `hvip` and any other timer interrupt directed to VS-level.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[[[ NOTE: I wrote this before I realized we haven't added any Sstc CSRs yet. Can you also add them so that this doesn't fall through the cracks? ]]]

VSTIP is altered by the Sstc extension.

19.2.2. Hypervisor Interrupt (hvip, hip, and hie) Registers
This extension modifies the description of the VSTIP/VSTIE bits in the hip/hie registers as follows:

Bits hip.VSTIP and hie.VSTIE are the interrupt-pending and interrupt-enable bits for VS-level timer interrupts. VSTIP is read-only in hip, and is the logical-OR of hvip.VSTIP and the timer interrupt signal resulting from vstimecmp (if vstimecmp is implemented). The hip.VSTIP bit, in response to timer interrupts generated by vstimecmp, is set and cleared by writing vstimecmp with a value that respectively is less than or equal to, or greater than, the current (time + htimedelta) value. The hip.VSTIP bit remains defined while V=0 as well as V=1.

This will require the function form of reset_value() on the field.

It will also require a sw_read() function for the CSR overall. Something like:

  sw_read(): |
    Bits<7> vstip_bit = 0;
    if (implemented?(ExtensionName::Sstc)) {
      if ((CSR[hvip].VSTIP == 1) | (CSR[vstimecmp].VALUE > (read_mtime() + CSR[htimedelta].DELTA)) {
        vstip_bit = 7'b1000000;
      }
    }

    Bits<3> vssip_bit = CSR[hvip].VSSIP == 0 ? 0 : 3'b100;
    # ...

    return lcofi_bit | sgeip_bit | vseip_bit | vstip_bit | vssip_bit;

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure ill add Sstc CSRs I believe these are they ?

  1. "Sstc" Extension for Supervisor-mode Timer Interrupts, Version 1.0.0 123
    16.1. Machine and Supervisor Level Additions 123
    16.1.1. Supervisor Timer Register (stimecmp) 123
    16.1.2. Machine Interrupt Registers (mip and mie) 124
    16.1.3. Supervisor Interrupt Registers (sip and sie) 124
    16.1.4. Machine Counter-Enable Register (mcounteren) 124
    16.2. Hypervisor Extension Additions 124
    16.2.1. Virtual Supervisor Timer Register (vstimecmp) 124
    16.2.2. Hypervisor Interrupt Registers (hvip, hip, and hie) 125
    16.2.3. Hypervisor Counter-Enable Register (hcounteren) 125
    16.3. Environment Config (menvcfg/henvcfg) Support

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Property sw_read() is not allowed.
getting schema error

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Shehrozkashif with just that info it's hard to give any meaningful input, can you share the sholw section where that happens?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sw_read() is a CSR property, not a CSR field property. Maybe that's why you're seeing a problem?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes paul is right! I can define this function sw_write(csr_value) but while adding sw_read():(the function derek wants me to add ) is not in fields
sw_read() so I think I have to add it as a property not in fields ?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

have to add [sw_read()] as a property not in fields ?

Correct. It should be in the YAML at the "top level" as a peer with other attributes like "name", "description", and (not to be too confusing) "fields".

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it, I’ll move sw_read() to the top level alongside name, description, and fields.

@@ -0,0 +1,46 @@
# yaml-language-server: $schema=../../../schemas/csr_schema.json
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This CSR needs a sw_read function that reflects the following (I've started a skeleton of it in the VSSIP comment):

SGEIP is read-only in hip, and is 1 if and only if the bitwise logical-AND of CSRs hgeip and hgeie is nonzero in any bit.

VSEIP is read-only in hip, and is the logical-OR of these interrupt sources:

  • bit VSEIP of hvip;
  • the bit of hgeip selected by hstatus.VGEIN; and
  • any other platform-specific external interrupt signal directed to VS-level.

VSTIP is read-only in hip, and is the logical-OR of hvip.VSTIP and any other platform-specific timer interrupt signal directed to VS-level.

VSSIP in hip is an alias (writable) of the same bit in hvip

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I presume this still needs to be added.

@Shehrozkashif Shehrozkashif force-pushed the H_CSRs branch 2 times, most recently from 5782dc7 to 075728f Compare May 13, 2025 18:50
@Shehrozkashif Shehrozkashif requested a review from ThinkOpenly June 5, 2025 17:58
@ThinkOpenly
Copy link
Collaborator

Shehrozkashif requested a review from ThinkOpenly 1 hour ago

@Shehrozkashif, I guess I'm not sure why you asked for a review. Are there specific changes you want me to review? There are still open review comments that need to be addressed, and several CI tests are failing. How would you like for me to help?

@Shehrozkashif
Copy link
Collaborator Author

@ThinkOpenly I m stuck with the CI tests failing ill address the remaining reviews asap

@dhower-qc
Copy link
Collaborator

smoke fails the vsie sw_read function because you have Sail syntax in an IDL block:

sw_read(): |
  let value = 0;

should be

sw_read(): |
  Bits<64> value = 0;


sw_read(): |
Bits<64> value = 0;
if (CSR[hideleg][2]) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

needs to convert to Bits:

  if ($bits(CSR[hideleg])[2]) {

multiple occurances of this.

@ThinkOpenly
Copy link
Collaborator

Referencing #565. There appears to be a parallel effort to address this same issue.

@codecov
Copy link

codecov bot commented Jul 22, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 43.26%. Comparing base (9cce5fc) to head (4419407).

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #617   +/-   ##
=======================================
  Coverage   43.26%   43.26%           
=======================================
  Files          10       10           
  Lines        4791     4791           
  Branches     1300     1300           
=======================================
  Hits         2073     2073           
  Misses       2718     2718           
Flag Coverage Δ
idlc 43.26% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@Shehrozkashif Shehrozkashif requested a review from dhower-qc July 24, 2025 08:30
@ThinkOpenly
Copy link
Collaborator

How shall we proceed here? @Shehrozkashif, do you want to finish this? Do we need to break it up any? I've lost context on what else is needed here. The CI tests are certainly not happy.

@Shehrozkashif
Copy link
Collaborator Author

@ThinkOpenly I can take this forward. I’ll start by fixing hip with the correct sw_read() placement and Sstc references, get CI passing there, and then apply the same pattern to the other CSRs. If it gets too big, I’ll split into smaller PRs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants